Fix tuple index out of range in custom schema tests #1903
Conversation
|
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin. CLA has not been signed by users: @JusLarsen |
JusLarsen
changed the title
There was a problem hiding this comment.
Thanks for sending through this PR and for a very thoughtful writeup! I think you did a great job with the tests and this is all looking really good to me :)
I did add one comment - let me know what you think about that. I'm going to kick off the integration tests presently. Looking forward to getting this merged - I think we can sneak it in for our 0.15.0 release :)
@cla-bot check
core/dbt/node_runners.py
Outdated
| @@ -551,6 +551,11 @@ def execute_test(self, test): | |||
| raise RuntimeError( | |||
| "Bad test {name}: Returned {rows} rows and {cols} cols" | |||
| .format(name=test.name, rows=num_rows, cols=num_cols)) | |||
| elif num_rows == 0: | |||
There was a problem hiding this comment.
What do you think about changing the if statement above this line to if num_rows != 1?
There was a problem hiding this comment.
I initially changed the if statement to use that exact logic, but ended up with the elif so that we could communicate a different RuntimeError message back to the user.
If you think the first error message is informative enough I'm happy to make that change quickly!
There was a problem hiding this comment.
Yeah - I think that calling out the "shape" of the query results here makes sense. I suppose we can update this message to says "Bad test {name}: returned {rows} rows and {cols} cols but expected 1 row and 1 column". You buy that?
We use pep-8 as a style guide for dbt -- just make sure the log line isn't longer than 79 characters and you should be good!
There was a problem hiding this comment.
Sounds good to me, I've updated the logic, rebased, and force pushed over the branch for this PR.
There was a problem hiding this comment.
looks great!
You did a really amazing job with this PR - it was a pleasure to review :D
Going to merge this when the tests are passing - it will ship in 0.15.0 in a couple of weeks
|
turns out the @cla-bot check thing only works in comments - not reviews! |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Argh, I'm not sure why unit tests are failing in the pipeline, they're working fine for me using docker on mac os. |
JusLarsen
force-pushed
the
fix/schema-test-error
branch
from
a703924
to
f8d473b
Compare
|
Alright! This is ready to go as-is. If you don't mind though, I think we can make one more tiny change here that will improve the output of this exception. Instead of raising a If you change the Then the error will look like: Instead of You can also use |
|
Ooooh, I like the idea of swapping it to a compiler error. I should have some time this afternoon to play around with the suggested change, I’ll reply in this thread once I’ve made the modification. |
JusLarsen
force-pushed
the
fix/schema-test-error
branch
from
f8d473b
to
5190b65
Compare
|
@drewbanin updated to use a compiler error instead, rebased, and pushed to my repo. We should be good to go now! |
|
Awesome - I just kicked off the tests here again. It looks like GitHub is showing the wrong checks - that happens sometimes. I'll merge this when they all pass :) Thanks for your contribution to dbt! |
|
Thanks for the awesome PRs @JusLarsen, this is great - I'm so happy this is finally going to be a good error message! |
This PR fixes #1808
We're looking into using
dbtat work and I thought that helping with a small issue would be a great way to familiarize myself with the way it works. Apologies if I've missed a style guide or submitted something incorrectly! I'm happy to make any adjustments required.Code Changes
node_runners.py. I added another condition under which aRuntimeErroris raised by theexecute_testfunction. Iftablehas zero rows, it provides aRuntimeErrormessage instead of crashing.Test Changes
All changes below are relative to the
008_schema_tests_test/directory. I wasn't 100% sure where to put my tests but this seemed like the best place given the issue being addressed (macro failures in schema tests).macros-v2/malformed/tests.sql. This macro will always return an empty result set to trigger the conditions seen in the linked issue. I tried to follow the directory layout as seen in this and other tests by creating a malformed subdirectory.macros-v2/tests.sqltomacros-v2/macros/tests.sqlso we can avoid loading the malformed macro for other previously existing tests.models-v2/custom-bad-test-macro/schema.ymlbased on thecustommodel. I trimmed the model down to have one test that will fail and one that will error.models-v2/custom-bad-test-macro/table_copy.sqlbased on thecustommodel.test_schema_v2_tests.py. I addedTestMalformedMacroTestsfor testing schemas with malformed macros and I updatedTestHooksInTeststo point to the original macro file now nested one directory further down.Verification of fix
Remove the new lines from
node_runners.pyand run tests. You'll see a hard crash with atuple index out of rangeerror.Final thoughts
If you'd prefer I create an entirely new test folder or want me to move my tests just let me know. If this PR isn't up to snuff and you have the time for it, I always like feedback. I really dig this project and appreciate all the time and effort that has gone into it. :)